Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

cable: external git info #1067

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

cable: external git info #1067

wants to merge 1 commit into from

Conversation

battlmonstr
Copy link
Collaborator

Problem:

A git commit sha or a branch name change causes relinking of executable targets with silkworm-buildinfo library even if nothing else has changed.

During development (e.g. local Debug builds) it is more important to have fast incremental builds than having the correct git commit sha and a branch name baked into the binary.

Solution:

Add arguments to cable_add_buildinfo_library:

  • GIT_DESCRIBE
  • GIT_BRANCH
  • GIT_ORIGIN_URL

If passed they override corresponding git commands.

In Silkworm we can hardcode them in Debug builds to avoid relinking and save time when doing small changes and git rebase/squash/reword operations.

See related Silkworm PR:
erigontech/silkworm#2473

Test

Silkworm git commit --amend --date=now && time make output before the change:

...
[11/70] Updating silkworm-buildinfo:
       Project Version:  0.1.0-dev+commit.11e1cbbc.dirty (prerelease)
       System Name:      darwin
       System Processor: arm64
       Compiler ID:      appleclang
       Compiler Version: 15.0.0.15000309
       Build Type:       debug
       Git Info:         //11e1cbbc69056575cae00370af1883e5f6504944 (dirty)
       Git Branch:       pr/test
       Git Origin URL:   [email protected]:erigontech/silkworm.git
       Timestamp:        2024-11-04T10:57:47
[68/69] Linking CXX executable cmd/dev/snapshots
make  104.37s user 9.23s system 509% cpu 22.279 total

Silkworm git commit --amend --date=now && time make output after the change:

...
[1/47] CableBuildInfo: updating gitinfo.txt
make  0.08s user 0.07s system 84% cpu 0.176 total

Note

I've also tried passing GIT_COMMIT_HASH directly to buildinfo.cmake instead of via gitinfo.cmake, but it didn't help, because gitinfo.cmake is regenerating gitinfo.txt, and gitinfo.txt is set as a dependency to buildinfo.c, so even though buildinfo.c contents was not changed, it was "touched" and silkworm-buildinfo library got rebuilt.

@battlmonstr battlmonstr requested a review from chfast November 4, 2024 11:10
@battlmonstr battlmonstr mentioned this pull request Nov 4, 2024
Merged
chfast
chfast reviewed Nov 4, 2024
View reviewed changes
Copy link
Member

@chfast chfast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have opinions here. I used git describe years ago because it seems ideal for this job: single command gets you all information and is based on the git tags as the versioning system. Anyway, I'm not sure it is worth to maintain the cable as the external dependency, so you may consider copying to your project.

Anyway, I can merge these changes if this helps. Just please describe the changes in the CHANGELOG at the beginning of the file.

@codecov Codecov
Copy link

codecov bot commented Nov 4, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.22%. Comparing base (ab2b4db) to head (a9c6b9a).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #1067   +/-   ##
=======================================
  Coverage   94.22%   94.22%           
=======================================
  Files         158      158           
  Lines       17066    17066           
=======================================
  Hits        16081    16081           
  Misses        985      985
Flag Coverage Δ
eof_execution_spec_tests 17.80% <ø> (ø)
ethereum_tests 26.52% <ø> (ø)
ethereum_tests_silkpre 19.12% <ø> (ø)
execution_spec_tests 20.23% <ø> (ø)
unittests 89.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@battlmonstr
cable: external git info
a9c6b9a 
Problem:

A git commit sha or a branch name change causes relinking of executable targets with silkworm-buildinfo library even if nothing else has changed.

During development (e.g. local Debug builds) it is more important to have fast incremental builds than having the correct git commit sha and a branch name baked into the binary.

Solution:

Add arguments to cable_add_buildinfo_library:
* GIT_DESCRIBE
* GIT_BRANCH
* GIT_ORIGIN_URL

If passed they override corresponding git commands.
@battlmonstr
Copy link
Collaborator Author

@chfast yes, git describe does the job. The problem is that with all the given parameters it becomes way too strict for our intents and purposes.

Who maintains Cable? Is it a 3rd-party dependency? I don't mind making it a direct dependency of silkworm, but prefer doing it as a separate change later, because I have a limited time budget for such work.

I've updated the CHANGELOG ✅

@chfast
Copy link
Member

chfast commented Nov 4, 2024

@chfast yes, git describe does the job. The problem is that with all the given parameters it becomes way too strict for our intents and purposes.

Who maintains Cable? Is it a 3rd-party dependency? I don't mind making it a direct dependency of silkworm, but prefer doing it as a separate change later, because I have a limited time budget for such work.

I've updated the CHANGELOG ✅

I do. https://github.com/ethereum/cable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chfast chfast chfast left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@battlmonstr @chfast